Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Corrected record-id values in example BODS files #727

Merged
merged 1 commit into from
Aug 26, 2024
Merged

Conversation

kd-ods
Copy link
Collaborator

@kd-ods kd-ods commented Aug 23, 2024

Overview

What does this pull request do?

At least four of the examples files on main (and in the 0.4 release) have some incorrect record ids. See #726

How can a reviewer test or examine your changes?

The best way to check the recordIds work is to see that that network graphs look as expected. See the related issue for visuals to test against.

Who is best placed to review it?

Myles

Closes #726

Translations

n/a

Documentation & Release

n/a

@kd-ods kd-ods mentioned this pull request Aug 23, 2024
7 tasks
@codemacabre
Copy link

codemacabre commented Aug 26, 2024

This all looks good in the visualisation tool except bods-package-entity-owning-entity.json, which uses exclusiveMaximum for the maximum share value. It looks like this is a new key in the share object, so I will need to work on making the visualisation tool compatible with that (although it doesn't appear to be documented explicitly in the changelog so I'm adding this comment here to check that it is correct before approving).

@kd-ods
Copy link
Collaborator Author

kd-ods commented Aug 26, 2024

Ah, yes. The changlelog has the teaser entry "Interest.share properties and requirements updated. Exact values and ranges are now represented in simpler ways."

To be precise: exclusiveMinimum and exclusiveMaximum properties did exist before in 0.3 etc, but they were booleans and acted as modifiers for the meaning of the minimum and maximum properties. From 0.4, exclusiveMinimum and exclusiveMaximum are now numbers. If publishers need to show that a share interest lies within a range, they use one of minimum and exclusiveMinimum and then one of maximum and exclusiveMaximum. (Do you think we should open up a visualiser issue where we can work on specifying the visualiser handling of the share object? It's not a simple matter, when the tool shouldn't get into the business of only rendering valid data.)

@codemacabre
Copy link

(Do you think we should open up a visualiser issue where we can work on specifying the visualiser handling of the share object? It's not a simple matter, when the tool shouldn't get into the business of only rendering valid data.)

I don't think it's necessary at this stage - the fix is a fairly straightforward conditional to check for (minimum or exclusiveMinimum) and (maximum or exclusiveMaximum) which should make valid data render correctly.

@kd-ods kd-ods merged commit 2e76fba into main Aug 26, 2024
4 checks passed
@kd-ods kd-ods deleted the kca-726-examples-fix branch August 26, 2024 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] - Some example files are incorrectly re-using recordId value for different entities
2 participants